-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
핵심기능 E2E 테스트 - 카테고리, 검색 #648
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생했습니다 헤인!
반복되는 로직을 utill로 분리하고 재사용하는 등 테스트 코드이지만 코드 품질을 신경쓰는 모습이 인상깊었어요.
몇가지 코멘트가 있어서 남겨봅니다.
추가로 merge 커밋을 고려해서 브랜치명도 좀 더 구체화하면 좋겠다는 생각이 드네요! (저한테도 하는 이야기... ㅋㅋ)
for (const char of rawCategoryName) { | ||
await page.keyboard.type(char); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 Locator.fill()로 입력하지 않고 page.keyboard.type으로 입력한 이유가 있나요? 다음 링크의 caution에서는 fill을 권장하는 것 같아서요!
https://playwright.dev/docs/api/class-keyboard#keyboard-type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locator.fill()은 복사붙여넣기 처럼 동작하기 때문에 16자 이상의 글자를 fill로 넣으면 아무것도 입력되지않아 해당 테스트에서 실패하였습니다. 🤣
(16자 이상의 글자를 복사붙여넣기하면 아예 복사붙여넣기가 되지 않는데, 그 이유는 validate 함수에서 아예 16자 이상의 입력을 막고있기 때문인 것 같습니다.)
따라서 15자 이후에 계속 입력하여도 15자까지만 카테고리가 생성된다는 것을 테스트하기 위해 page.keyboard.type를 사용하였습니다!
frontend/e2eTests/testUtils.ts
Outdated
export const waitForSuccess = async ({ page, url }: WaitForSuccessProps) => { | ||
await page.waitForResponse((response) => response.url().includes(url) && response.status() === 200); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 유틸함수로 만드는 것 좋네요!
한가지 반영하지 않아도 되는 제안) url이 처음에는 route에 쓰이는 url인줄 알고 잘못 썼었는데, apiUrl 같은 네이밍이나 유틸함수이니까 JsDocs로 타입에 대한 설명을 붙이면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아주 좋은 제안입니다! apiUrl
로 변경하겠습니다~!!
frontend/e2eTests/search.spec.ts
Outdated
test.beforeEach(async ({ page }) => { | ||
await loginToCodezap({ page, username: 'll', password: 'llll1111' }); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한가지 고민, 이런 테스트용 아이디를 소스코드에 노출해도 괜찮을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호... 이 부분 env로 관리하는게 좋겠네요!!
frontend/e2eTests/category.spec.ts
Outdated
const deleteCategory = async ({ page, newCategoryName }: Props) => { | ||
await page.getByRole('button', { name: '카테고리 편집' }).click(); | ||
|
||
await page.getByText(newCategoryName).nth(1).hover(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 nth(1) 같은 경우엔 변수에 할당해서 명시적으로 무엇에 hover하는지 하면 좋겠네요!
ex) const deleteButton = page.getByText(newCategoryName).nth(1);
…username => id 로 변경
⚡️ 관련 이슈
📍주요 변경 사항
1. 카테고리 관련 E2E 테스트 추가
2. 검색 관련 E2E 테스트 추가
테스트
를 입력하면테스트
가 내용에 포함된 템플릿 목록을 확인할 수 있다.ㅁㅅㅌㅇ
를 입력할 경우검색 결과가 없습니다
가 나온다.🎸기타
testUtils
라는 파일 내에loginToCodezap
로 만들어 사용하였습니다. 마위가templateActions
에 만든 함수와 중복되는 로직이기 때문에 논의 후 하나로 통일해야 합니다.